Add raw_socket_proxy to directly proxy websockets to TCP/unix sockets
#447
Conversation
|
Thanks for submitting your first pull request! You are awesome! 🤗 |
|
Thanks for the PR, and sorry for the delay in reviewing. I like the idea of having a self contained pure Python implementation! I can't guarantee we'll merge it since To help with review would you mind rebasing this onto |
This incorporates the functionality of websockify. In doing so, it eliminates an extra proxy layer and process, improving performance with a fairly small amount of code. Fixes jupyterhub#75. This also generalizes and combines the _make_namedproxy_handler and _make_supervisedproxy_handler functions into a single case to allow more flexibility.
|
Thanks, yeah I understand the concerns, but having worked on the websockify code as well, I can at least say that this PR is overall much simpler and easier to maintain, and I'm happy to help do so. I've rebased, and tested again on the updated version (with jupyter-remote-desktop-proxy) and things look okay. Probably there should be some automated tests for this, too -- I can look into that if there's interest in moving forward. |
|
Thanks! Tests would be great if we go ahead but hold off for now. I'll try out your changes locally first, feel free to bump this issue if I forget! |
|
Bump reminder, @manics :) |
There was a problem hiding this comment.
Sorry for the delay in reviewing this. The code looks OK to me, but I don't have much experience with websockets so ideally it could do with a second review.
I've tested this with jupyter-remote-desktop-proxy and it works well!
diff --git a/jupyter_remote_desktop_proxy/setup_websockify.py b/jupyter_remote_desktop_proxy/setup_websockify.py
index c6b1f4b..f25801e 100644
--- a/jupyter_remote_desktop_proxy/setup_websockify.py
+++ b/jupyter_remote_desktop_proxy/setup_websockify.py
@@ -12,8 +12,6 @@ def setup_websockify():
raise RuntimeError(
"vncserver executable not found, please install a VNC server"
)
- if not which('websockify'):
- raise RuntimeError("websockify executable not found, please install websockify")
# TurboVNC and TigerVNC share the same origin and both use a Perl script
# as the executable vncserver. We can determine if vncserver is TigerVNC
@@ -30,16 +28,10 @@ def setup_websockify():
is_tigervnc = "tigervnc" in vncserver_file.read().casefold()
if is_tigervnc:
- # Make a secure temporary directory for sockets that is only readable,
- # writeable, and searchable by our uid - TigerVNC can listen to a Unix
- # socket!
- sockets_dir = tempfile.mkdtemp()
- sockets_path = os.path.join(sockets_dir, 'vnc-socket')
-
- websockify_args = ['--unix-target', sockets_path]
- vnc_args = [vncserver, '-rfbunixpath', sockets_path]
+ unix_socket = True
+ vnc_args = [vncserver, '-rfbunixpath', "{unix_socket}"]
else:
- websockify_args = []
+ unix_socket = False
vnc_args = [vncserver, '-localhost', '-rfbport', '{port}']
if not os.path.exists(os.path.expanduser('~/.vnc/xstartup')):
@@ -58,18 +50,13 @@ def setup_websockify():
)
return {
- 'command': [
- 'websockify',
- '--verbose',
- '--heartbeat=30',
- '{port}',
- ]
- + websockify_args
- + ['--', '/bin/sh', '-c', f'cd {os.getcwd()} && {vnc_command}'],
+ 'command': ['/bin/sh', '-c', f'cd {os.getcwd()} && {vnc_command}'],
'timeout': 30,
'new_browser_window': True,
# We want the launcher entry to point to /desktop/, not to /desktop-websockify/
# /desktop/ is the user facing URL, while /desktop-websockify/ now *only* serves
# websockets.
"launcher_entry": {"title": "Desktop", "path_info": "desktop"},
+ "unix_socket": unix_socket,
+ "websockify": True,
}@yuvipanda what do you think?
|
I very much like the idea of not depending on websockify for TCP streams. This would allow us to package remote-desktop-proxy in conda-forge more easily too. I've re-opened #75 and will look through the code now. |
There was a problem hiding this comment.
I'll do another round next week, but left two comments here!
I also think we need to add a test - perhaps similar to the existing websocket test, but with a simple TCP server?
Am really excited to get this in, as it would allow us to do a bunch of other cool stuff elsewhere :) For example, it would enable proper ssh forwarding in https://github.com/yuvipanda/jupyterhub-ssh...
Thank you for working on this, and your patience as we review this, @dylex!
There was a problem hiding this comment.
I spent a little more time looking at this, and the overall structure seems fine to me. It does need a test (perhaps modeled after what our current websocket tests do, but with raw TCP). After that + the naming stuff, happy to merge.
Thank you for working through this, @dylex!
|
Okay, I think everything has been addressed now. Option is now named |
yuvipanda
changed the title
raw_socket_proxy to directly proxy websockets to TCP/unix sockets
|
Alright, thank you very much for your work, @dylex! And sorry about the slow review. I hope to see more contributions from you in the future too <3 |
jupyter-server-proxy 4.3.0 includes jupyterhub/jupyter-server-proxy#447
jupyter-server-proxy 4.3.0 includes jupyterhub/jupyter-server-proxy#447
This incorporates the functionality of websockify by allowing websockets to be proxied to simple TCP or unix socket streams.
While I understand that websockify already provides a working solution to this problem (see discussion on #75), this seems to have a number of benefits, mainly that it eliminates the extra proxy layer and process that websockify imposes, improving latency and performance overall. It does so with a fairly small amount of code, much smaller and ismpler than websockify itself.
To simplify the instantiation of proxy handlers, this also generalizes and combines the
_make_namedproxy_handlerand_make_supervisedproxy_handlerfunctions into a single_make_proxy_handlerthat dynamically determines the class to use, improving flexibility and reducing code duplication, at the cost of a little complexity.